fix(rds): Correct ARN in IAM policy for IAM database access #25141

akash1810 · 2023-04-15T08:26:32Z

The IAM policy for IAM database access takes the form of:

arn:aws:rds-db:region:account-id:dbuser:DbiResourceId/db-user-name

Following aws-cloudformation/cloudformation-coverage-roadmap#105, this change updates the ARN used in grantConnect to this format.

Additionally, update the signature of grantConnect to take an optional dbUser, which is defaulted to the master username of the database, obtained via the available Secret.

This signature change also matches grantConnect in DatabaseProxy. See #12416.

Closes #11851.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

akash1810 · 2023-04-15T21:35:44Z

Clarification Request.

akash1810 · 2023-04-15T21:45:53Z

packages/aws-cdk-lib/aws-rds/lib/instance.ts

@@ -130,6 +147,7 @@ export abstract class DatabaseInstanceBase extends Resource implements IDatabase
      public readonly instanceEndpoint = new Endpoint(attrs.instanceEndpointAddress, attrs.port);
      public readonly engine = attrs.engine;
      protected enableIamAuthentication = true;
+      public readonly instanceResourceId = attrs.instanceResourceId;


This is a new, required property. Is it a breaking change for fromDatabaseInstanceAttributes?

This is a new, required property. Is it a breaking change for fromDatabaseInstanceAttributes?

Ahh the AWS CodeBuild report suggests it is breaking:

err - IFACE aws-cdk-lib.aws_rds.DatabaseInstanceAttributes: newly required property 'instanceResourceId' added: input to aws-cdk-lib.aws_rds.DatabaseInstanceBase.fromDatabaseInstanceAttributes [strengthened:aws-cdk-lib.aws_rds.DatabaseInstanceAttributes] Some packages seem to have undergone breaking API changes. Please avoid.

Advice on how to proceed welcomed 🙏🏽.

I think you can change this to an optional property in IDatabaseInstance and DatabaseInstanceBase and fromDatabaseInstanceAttributes.

I think you can change this to an optional property in IDatabaseInstance and DatabaseInstanceBase and fromDatabaseInstanceAttributes.

Done in ce630ec.

I'm not a huge fan of this as it means one has to ensure the property is not undefined when accessing it, which adds bloat IMO:

const db = new DatabaseInstance(); const resourceId: string | undefined = db.instanceResourceId; if (!resourceId) { // unhappy path } else { // happy path }

Alternatively, one can use non-null assertion, however I've worked in projects where this would produce a lint warning.

const db = new DatabaseInstance(); const resourceId: string = db.instanceResourceId!;

Additionally, this being optional makes it the only optional property¹. Not a problem in general, however might be an indication of not being desired?

Footnotes

enableIamAuthentication doesn't really count, as an optional boolean still has two values: true = true, false|undefined = false. ↩

Yeah it's not great, but it's the only way to avoid breaking changes.

akash1810 · 2023-04-20T06:34:39Z

Exemption Request

Unit tests have been updated, and an additional one added.

Running the tests via yarn --cwd packages/aws-cdk-lib build+test doesn't produce any git diff, so not sure what integration and/or snapshot files need updating.

The IAM policy for IAM database access takes the form of: ``` arn:aws:rds-db:region:account-id:dbuser:DbiResourceId/db-user-name ``` Update the ARN used in `grantConnect` to this format. Additionally, update the signature of `grantConnect` to take an optional `dbUser`, which is defaulted to the master username of the database. This signature change also matches `grantConnect` in `DatabaseProxy`. See: - https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/UsingWithRDS.IAMDBAuth.IAMPolicy.html - aws-cloudformation/cloudformation-coverage-roadmap#105

To avoid a breaking change, make `instanceResourceId` an optional prop.

corymhall · 2023-04-20T11:30:21Z

The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
PRs must pass status checks before we can provide a meaningful review.

Integration tests are now added in @aws-cdk-testing/framework-integ. In this case I think the integ test would only be useful if we created an assertion test where something (i.e. lambda function) was able to connect to the database, but I think the effort to create that case is too large so I'm exempting it.

corymhall

🚀

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2023-04-20T11:31:15Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

corymhall · 2023-04-20T11:33:28Z

@akash1810 you need to authorize modification on the head branch in order for mergify to merge this.

akash1810 · 2023-04-20T11:42:14Z

@akash1810 you need to authorize modification on the head branch in order for mergify to merge this.

Ah. Done that now. Thanks! Do I need to do anything to re-run mergify?

corymhall · 2023-04-20T11:52:09Z

@Mergifyio requeue

mergify · 2023-04-20T11:52:12Z

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

gitpod-io · 2023-04-20T12:27:41Z

aws-cdk-automation · 2023-04-20T12:50:56Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: fc0aaa2
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-04-20T12:51:21Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

ChrisLahaye · 2023-05-11T08:26:37Z

We are experiencing some issues after upgrading to v2.77.0 which I believe is due to this change.

When using databaseInstance.grantConnect(iamRole) the following error is thrown:

Error: Resolution error: Resolution error: Resolution error: Resolution error: Synthing a secret value to Resources/${Token[xxx.ServiceAccount.Role.DefaultPolicy.Resource.LogicalID.2000]}/Properties/policyDocument/Statement/0/Resource. Using a SecretValue here risks exposing your secret. Only pass SecretValues to constructs that accept a SecretValue property, or call AWS Secrets Manager directly in your runtime code. Call 'secretValue.unsafeUnwrap()' if you understand and accept the risks.

I think this happens due to the following new code:

dbUser = this.secret.secretValueFromJson('username').toString();

Perhaps this should be unsafeUnwrap instead of toString?

nick-asher · 2023-07-13T10:31:13Z

Don't suppose there's any appetite to populate this property when fetching a database via FindDatabases()? There's no clear way defined to actually populate this property in this commit unless I'm missing something? The one comment in the docs about it being logged in CloudTrail doesn't feel like a sensible way to fetch the Id.

aws-cdk-automation requested a review from a team April 15, 2023 08:26

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Apr 15, 2023

aws-cdk-automation previously requested changes Apr 15, 2023

View reviewed changes

akash1810 changed the title ~~fix(aws-rds): Correct ARN in IAM policy for IAM database access~~ fix(rds): Correct ARN in IAM policy for IAM database access Apr 15, 2023

akash1810 marked this pull request as ready for review April 15, 2023 21:34

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Apr 15, 2023

akash1810 commented Apr 15, 2023

View reviewed changes

corymhall self-assigned this Apr 17, 2023

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Apr 20, 2023

akash1810 added 2 commits April 20, 2023 07:45

refactor(rds): Make instanceResourceId prop optional

ce630ec

To avoid a breaking change, make `instanceResourceId` an optional prop.

corymhall added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 20, 2023

corymhall approved these changes Apr 20, 2023

View reviewed changes

Merge branch 'main' into fix-grantConnect

fc0aaa2

mergify bot merged commit 227ea09 into aws:main Apr 20, 2023

akash1810 deleted the fix-grantConnect branch April 20, 2023 12:58

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rds): Correct ARN in IAM policy for IAM database access #25141

fix(rds): Correct ARN in IAM policy for IAM database access #25141

akash1810 commented Apr 15, 2023 •

edited

Loading

aws-cdk-automation left a comment

akash1810 commented Apr 15, 2023

akash1810 Apr 15, 2023

akash1810 Apr 15, 2023 •

edited

Loading

corymhall Apr 17, 2023

akash1810 Apr 20, 2023 •

edited

Loading

corymhall Apr 20, 2023

akash1810 commented Apr 20, 2023 •

edited

Loading

corymhall commented Apr 20, 2023

corymhall left a comment

mergify bot commented Apr 20, 2023

corymhall commented Apr 20, 2023

akash1810 commented Apr 20, 2023 •

edited

Loading

corymhall commented Apr 20, 2023

mergify bot commented Apr 20, 2023

gitpod-io bot commented Apr 20, 2023

aws-cdk-automation commented Apr 20, 2023

mergify bot commented Apr 20, 2023

ChrisLahaye commented May 11, 2023

nick-asher commented Jul 13, 2023

fix(rds): Correct ARN in IAM policy for IAM database access #25141

fix(rds): Correct ARN in IAM policy for IAM database access #25141

Conversation

akash1810 commented Apr 15, 2023 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

akash1810 commented Apr 15, 2023

akash1810 Apr 15, 2023

Choose a reason for hiding this comment

akash1810 Apr 15, 2023 • edited Loading

Choose a reason for hiding this comment

corymhall Apr 17, 2023

Choose a reason for hiding this comment

akash1810 Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Footnotes

corymhall Apr 20, 2023

Choose a reason for hiding this comment

akash1810 commented Apr 20, 2023 • edited Loading

corymhall commented Apr 20, 2023

corymhall left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 20, 2023

corymhall commented Apr 20, 2023

akash1810 commented Apr 20, 2023 • edited Loading

corymhall commented Apr 20, 2023

mergify bot commented Apr 20, 2023

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

gitpod-io bot commented Apr 20, 2023

aws-cdk-automation commented Apr 20, 2023

AWS CodeBuild CI Report

mergify bot commented Apr 20, 2023

ChrisLahaye commented May 11, 2023

nick-asher commented Jul 13, 2023

akash1810 commented Apr 15, 2023 •

edited

Loading

akash1810 Apr 15, 2023 •

edited

Loading

akash1810 Apr 20, 2023 •

edited

Loading

akash1810 commented Apr 20, 2023 •

edited

Loading

akash1810 commented Apr 20, 2023 •

edited

Loading